Conversation
|
Please rebase pull request. |
It is a simplified constructor for dynamic validator used in the ARO operator.
m1kola
force-pushed
the
refactor_dynamic_validator
branch
from
6d2dffc to
b4730a8
Compare
m1kola
added
priority-low
There was a problem hiding this comment.
This looks good. I like the separation of the dynamic validator. One quick question about the constructor: subscriptionID is not necessary it seems.
and the necessary clients for the ServicePrincipalValidtor to perform the check in the operator.
Edit: subscriptionID is necessary to create clients from functions in dynamic constructor 👍🏽
Edit 2: The clientset is nil, because the ValidateServicePrincipal doesn't use any of them, so the subscriptionID is not necessary in this constructor.
| }, nil | ||
| } | ||
|
|
||
| func NewServicePrincipalValidator(log *logrus.Entry, azEnv *azureclient.AROEnvironment, subscriptionID string, authorizerType AuthorizerType) (ServicePrincipalValidator, error) { |
There was a problem hiding this comment.
| func NewServicePrincipalValidator(log *logrus.Entry, azEnv *azureclient.AROEnvironment, subscriptionID string, authorizerType AuthorizerType) (ServicePrincipalValidator, error) { | |
| func NewServicePrincipalValidator(log *logrus.Entry, azEnv *azureclient.AROEnvironment, authorizerType AuthorizerType) (ServicePrincipalValidator, error) { |
We are not using the subscriptionID anywhere in this function. I don't think this is necessary for the constructor.
Is this missing the clients necessary to do the ValidateServicePrincipal check that it had when it was a full dynamic validator?
There was a problem hiding this comment.
This makes sense, disregard.
Edit: See below comment
There was a problem hiding this comment.
No, @troy0820, I think you were right. subscriptionID is not used anywhere. It is passed into the function and then it dies.
There was a problem hiding this comment.
@nilsanderselde The subscriptionID is passed into this constructor to fulfill the functions that create the rest of the dynamic type, it's just not explicitly set because this happens with functions below on the dynamic struct fields.
Edit: I don't believe that it uses these clients, so the subscriptionID isn't necessary.
providers: features.NewProvidersClient(azEnv, subscriptionID, authorizer),
spComputeUsage: compute.NewUsageClient(azEnv, subscriptionID, authorizer),
spNetworkUsage: network.NewUsageClient(azEnv, subscriptionID, authorizer),
permissions: authorization.NewPermissionsClient(azEnv, subscriptionID, authorizer),
virtualNetworks: newVirtualNetworksCache(network.NewVirtualNetworksClient(azEnv, subscriptionID, authorizer)),
diskEncryptionSets: compute.NewDiskEncryptionSetsClient(azEnv, subscriptionID, authorizer),
It doesn't need to be explicitly set because the subscriptionID is necessary for the above functions in the creation of this type.
troy0820
dismissed
their stale review
The subscriptionID is necessary to build the clients to perform the check. 👍🏽
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
nilsanderselde
left a comment
nilsanderselde
left a comment
There was a problem hiding this comment.
Param subscriptionID is not used anywhere.
troy0820
added
question
troy0820
dismissed
their stale review
Needs clarification with @nilsanderselde's question.
|
PR accidentaly got merged too early. I'm addressing feedback in #1619 |
4 participants
#1595 needs to be merged first.
Adds a simplified constructor for dynamic validator used in the ARO operator.